Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Truncate the statefulset pvc name to be max 63 chars #799

Merged
merged 2 commits into from
Nov 8, 2021

Conversation

tjhiggins
Copy link
Contributor

@tjhiggins tjhiggins commented Oct 20, 2021

Changes proposed in this PR:

How I expect reviewers to test this PR:

  • Run the server-statefulset.bats unit tests

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 20, 2021

CLA assistant check
All committers have signed the CLA.

@tjhiggins
Copy link
Contributor Author

Hmm not sure why the test-control-plane test failed.

I ran helm template --debug . and it generated valid templates.

@david-yu david-yu linked an issue Oct 26, 2021 that may be closed by this pull request
@david-yu
Copy link
Contributor

Hi @tjhiggins I reran the tests from the failure, it looks like it was a unit test that failed that prevented the acceptance tests from running. Thank you for the contribution!

@tjhiggins
Copy link
Contributor Author

Hi @tjhiggins I reran the tests from the failure, it looks like it was a unit test that failed that prevented the acceptance tests from running. Thank you for the contribution!

Which unit test? The only failing test I see now is the dev-upload-docker. Confused if I need to do anything.

@david-yu
Copy link
Contributor

david-yu commented Oct 28, 2021

@tjhiggins There was a unit test that failed so I re-ran the tests from CircleCI starting from that failure. The unit tests can be flaky sometimes. I believe our dev uploads are currently failing which is ok. We'd need someone probably to take a look and review this PR formally, we've been a little behind on PR reviews due to our beta release this week.

Could you perhaps show us what the output looks like for kubectl get pvc with this PR so we can see if the PVC names look ok? That would be helpful to get us to review this quicker.

@tjhiggins
Copy link
Contributor Author

@david-yu

tj:consul (798-truncate-statefulset-pvc)$ kubectl get pvc
NAME                                  STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
data-default-consul-consul-server-0   Bound    pvc-fd364619-f97d-4cac-8b97-b8a6cc3009fd   10Gi       RWO            hostpath       53s
data-default-consul-consul-server-1   Bound    pvc-7cf89cf1-2f89-4602-9943-78929b2229c4   10Gi       RWO            hostpath       53s
data-default-consul-consul-server-2   Bound    pvc-5405d834-e29b-4516-9026-3907e5b449bd   10Gi       RWO            hostpath       53s

ottaviohartman pushed a commit to ottaviohartman/consul-k8s that referenced this pull request Nov 3, 2021
* Don't run them in parallel so that we only get one slack notification
* Add gotestsum summary
@lkysow
Copy link
Member

lkysow commented Nov 5, 2021

Looks great! Can you write a Helm test for it?

@tjhiggins
Copy link
Contributor Author

@lkysow done

@test "server/StatefulSet: truncation for namespace > 58 chars" {
cd `chart_dir`
local actual=$(helm template \
-n really-really-really-really-really-really-really-long-namespace \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!! Thanks so much.

@lkysow lkysow merged commit 5778495 into hashicorp:main Nov 8, 2021
@lkysow lkysow mentioned this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consul server statefulset volume name isn't truncated
4 participants